-
Notifications
You must be signed in to change notification settings - Fork 109
Use the do {} while (false) idiom for BOOST_TEST_THROWS() and BOOST_TEST_NO_THROW() #205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the do {} while (false) idiom for BOOST_TEST_THROWS() and BOOST_TEST_NO_THROW() #205
Conversation
|
This is technically a breaking change because the semicolon is now required, whereas it wasn't before. But it looks like all our uses have a semicolon. The backslash at the last line shouldn't be present. We should remove it from BOOST_TEST_THROWS, instead of adding it to BOOST_TEST_NO_THROW. |
I thought the convention was to terminate with a Didn't notice this hadn't been done for |
…EST_NO_THROW()
Reason: Common hygiene for multi-line macros. Makes things like
if (condition)
BOOST_TEST_THROWS(...);
else
...
work correctly. Also, it doesn't generate an empty statement (which
compilers may warn about) when the user adds a semicolon (which they
usually do).
459d57d to
8a352e2
Compare
Yeah :-/ |
|
Sigh. Which version of MSVC? I don't get that warning with MSVC 2022 and |
|
Up to msvc-12.0 according to Appveyor. |
|
Hmm. We might try with |
|
Just add |
0cfc658 to
d30689c
Compare
|
BOOST_TEST_xxx is a public macro name. Please use BOOST_LWT_DETAIL_xxx for implementation details. |
d30689c to
a931f06
Compare
|
One other alternative we could try is |
|
That doesn't play well with the semicolon (the one after the macro invocation). |
48268d5 to
7ffb4f8
Compare
7ffb4f8 to
4ee817d
Compare
|
Everything fails, probably because you're missing a semicolon here |
|
|
4ee817d to
1ed1e04
Compare
|
Do you prefer that? (Why?) |
|
It's idiomatic because that's what Nowadays |
|
I don't have a strong opinion about that. I can change it, if you like. (About |
|
Yes please, let's go with |
Reason: See the new code comment.
1ed1e04 to
40fda50
Compare
|
No, looks good to me. |
|
Looks like this is the only such occurrence though. |
|
@Lastique: That changelog entry is a bit wrong: With the previous implementation (without was a syntax error. The warnings you hint at, instead, were for the |
|
The previous definition was a direct BTW, given that the macros are now conditionally defined depending on Anyway, if you can provide a better text in the changelog, please create a PR. |
Yes.
Yes, it can be used only if MSVC < 2015 is of no concern :-(. That MSVC warning ruined everything. I wish I could withdraw this PR. |
|
Fixed in 7b2c714. |
Reason: Common hygiene for multi-line macros. Makes things like
work correctly. Also, it doesn't generate an empty statement (which compilers may warn about) when the user adds a semicolon (which they usually do).